fix: Skip double finalisation if Contract Create exceptionally halted #21955
Conversation
Signed-off-by: Stanimir Stoyanov <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #21955 +/- ##
============================================
- Coverage 70.83% 70.82% -0.01%
- Complexity 24457 24460 +3
============================================
Files 2680 2680
Lines 104501 104502 +1
Branches 10968 10968
============================================
- Hits 74020 74017 -3
- Misses 26452 26457 +5
+ Partials 4029 4028 -1
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Signed-off-by: Stanimir Stoyanov <[email protected]>
| * @param to The recipient's EVM address. | ||
| * @param amount The number of tokens to mint (using 0 decimals, as defined). | ||
| */ | ||
| function mintReward(address to, int64 amount) external onlyOwner { |
There was a problem hiding this comment.
Is this method used?
There was a problem hiding this comment.
as mentioned above this contract was only used to reproduce the issue
| * It is NOT an ERC-20 token itself. | ||
| * It is given the Supply Key of the HTS token so it can mint rewards. | ||
| */ | ||
| contract HushSenseManager is Ownable { |
There was a problem hiding this comment.
What's HushSense? Why do we need this?
There was a problem hiding this comment.
This is a contract that was causing one of the warnings we observed on mainnet and is only used to reproduce the issue
| // to keep the action stack in sync with the message frame stack. | ||
| if (hasActionSidecarsEnabled(frame) && haltReason.isPresent()) { | ||
| // We skip finalizing for empty action stack, as those produce warnings. | ||
| if (hasActionSidecarsEnabled(frame) && haltReason.isPresent() && !actionStack.isEmpty()) { |
There was a problem hiding this comment.
Does this really address the root cause? Do we know why the action stack is empty in the first place?
There was a problem hiding this comment.
The root cause is running out of gas during execution of contract create. In this case the EVM is finalising correctly but we try to force a double thus the warnings.
There was a problem hiding this comment.
Another case that was hitting this path was trying to deploy on a occupied address which was also halting with insufficient gas
There was a problem hiding this comment.
Thanks @stoyanov-st , but why are we calling traceAccountCreationResult twice? Shouldn't it be called only once?
Description:
We observed WARN logs for prematurely empty Action Stack with the latest version of CN.
This PR aims to skip the double finalisation that happens in the case of exceptionally halting contract create ops.
Related issue(s):
Fixes #21821
Notes for reviewer:
Checklist